Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preview v5: Follow GOV.UK Frontend coding standards #2961

Merged
merged 10 commits into from
Aug 1, 2023

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Jul 20, 2023

This PR includes:

  1. Improved element instanceof checks
  2. Removal of component init() methods
  3. Fixes type checked .parentElement versus parentNode
  4. Fixes type mismatch .setAttribute() with boolean values

I've also added a TypeScript compiler tsconfig.json config for code autocomplete etc

No compiler GitHub Actions checks are included, but follows what we did in:

@colinrotherham colinrotherham changed the base branch from main to v5-classes July 20, 2023 08:31
@netlify
Copy link

netlify bot commented Jul 20, 2023

You can preview this change here:

Name Link
🔨 Latest commit 25c911c
🔍 Latest deploy log https://app.netlify.com/sites/govuk-design-system-preview/deploys/64c8b59016f3e10008a33686
😎 Deploy Preview https://deploy-preview-2961--govuk-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@colinrotherham colinrotherham added this to the v5.0 milestone Jul 20, 2023
@colinrotherham colinrotherham changed the title Pre-release: Follow GOV.UK Frontend coding standards Preview v5: Follow GOV.UK Frontend coding standards Jul 25, 2023
@colinrotherham colinrotherham force-pushed the v5-types branch 2 times, most recently from 01a88fd to d1bdf84 Compare July 26, 2023 13:28
Base automatically changed from v5-classes to release/5.0 July 27, 2023 16:52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything that's in the src directory will get copied to deploy/public on build, which means that this file will get served at https://design-system.service.gov.uk/tsconfig.json.

Not a major issue but doesn't seem ideal either – any way we could move these into the base config but only apply them to src?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look at this

Good point! I've filtered it out using Metalsmith's .ignore()

We could move it to the base but we need to scope only ./src to "lib": ["ESNext", "DOM"]

Will likely want the compiler to check ./lib and ./tasks in future, but without DOM libraries

@@ -16,6 +16,6 @@ export default function loadAnalytics () {
j.async = true
j.src = `https://www.googletagmanager.com/gtm.js?id=${i}${dl}`
document.head.appendChild(j)
})(window, document, 'script', 'dataLayer', 'GTM-53XG2JT')
})(window, document, /** @type {const} */ ('script'), 'dataLayer', 'GTM-53XG2JT')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this isn't really 'our' script, would it make more sense to turn off all type checking for the snippet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with that. Added // @ts-nocheck like we did for polyfills in the past


const $successNotification = $module.querySelector('.js-cookies-page-success')
if (!($successNotification instanceof HTMLElement)) {
return this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we might follow up and change this to throw errors, but at the minute this has potential to trip us up by silently swallowing errors.

Given that $successNotification is only used within showSuccessNotification, could we move the lookup there, and return early if the banner isn't present?

That way at least the rest of the cookie page functionality will continue to work if the banner is missing for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thinking

I've copied what we do elsewhere for optional selectors

const $successNotification = $module.querySelector('.js-cookies-page-success')
if ($successNotification instanceof HTMLElement) {
  this.$successNotification = $successNotification
}

☝️ Avoid appending to class property unless checks pass, but check again in showSuccessNotification()

@colinrotherham colinrotherham force-pushed the v5-types branch 3 times, most recently from 67e5a4e to 399f2fb Compare August 1, 2023 07:31
@colinrotherham colinrotherham merged commit efe741e into release/5.0 Aug 1, 2023
@colinrotherham colinrotherham deleted the v5-types branch August 1, 2023 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants